Repository navigation
Conversation
8cb9741 to
52dbb61
Compare
…ing the storage read
52dbb61 to
7903090
Compare
LukasMod
changed the title
|
@fabioh8010
cc @TaduJR |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
Assignment here @chuckdries Thanks. Reviewing... |
TaduJR
left a comment
TaduJR
left a comment
There was a problem hiding this comment.
Thanks @LukasMod. Checking the key index first and removing ?? undefined both look good.
Blocking: Double checked my previous comment and the re-check at :34, hides the data loss this log is for. Details inline.
The description's "first member with empty storage" case is common on web. A removal in another tab reaches the sync handler as undefined, and cache.set(key, undefined) keeps the key indexed but clears its nullish mark, so it looks like a lost value. Its next re-read mutes the collection, and if that tab sets the key again first, it logs a false alarm the re-check misses.
Marking the key nullish when cache.set gets null/undefined fixes both and keeps the key indexed, so clear still removes it. Dropping it from the index instead would leave it in storage after sign-out when a stale sync read lands after a local write. This was already in #850, so a follow-up is fine:
// OnyxCache.set
if (value === null || value === undefined) {
this.nullishStorageKeys.add(key);
delete this.storageMap[key];
// ...
}
this.nullishStorageKeys.delete(key);|
|
||
| function reportCacheMissIfStorageHasValue(key: OnyxKey): void { | ||
| if (OnyxKeys.isRamOnlyKey(key) || !cache.getAllKeys().has(key) || cache.hasNullishStorageKey(key)) { | ||
| if (!cache.getAllKeys().has(key) || OnyxKeys.isRamOnlyKey(key) || cache.hasNullishStorageKey(key)) { |
There was a problem hiding this comment.
This only works while undefined isn't in the key index. Onyx.set(undefined, 1) adds it before it throws, and after that getState(undefined) throws for every reader. It also returns the value of a key named 'undefined'. Checking the key where it enters Onyx, as update does, fixes both:
// OnyxKeys.ts
function isValidKey(key: unknown): key is string {
return typeof key === 'string';
}
// Top of Onyx.set and Onyx.merge
if (!OnyxKeys.isValidKey(key)) {
Logger.logInfo(`Invalid ${typeof key} key provided in Onyx set. Key must be of type string. Skipping this operation.`);
return Promise.resolve();
}
// Top of getState, which every read goes through
if (!OnyxKeys.isValidKey(key)) {
return undefined as OnyxValue<TKey>;
}| Storage.getItem(key) | ||
| .then((value) => { | ||
| if (value === undefined || value === null) { | ||
| if (value === undefined || value === null || cache.get(key) !== undefined) { |
There was a problem hiding this comment.
This skips the log when a write lands on a lost value. The cache has lost {a, b}, and merge(key, {c: 3}) builds on the empty cache. On web it overwrites storage, so the read sees {c: 3} in the cache and nothing is logged.
| if (value === undefined || value === null || cache.get(key) !== undefined) { | |
| if (value === undefined || value === null) { |
| it('should not report when the value reached the cache before storage answered', async () => { | ||
| await StorageMock.setItem(ONYXKEYS.TEST_KEY, 'from-storage'); | ||
| forgetValueButKeepKey(ONYXKEYS.TEST_KEY); | ||
|
|
||
| onyxSubscriptionManager.getState(ONYXKEYS.TEST_KEY); | ||
| cache.set(ONYXKEYS.TEST_KEY, 'from-storage'); | ||
| await waitForPromisesToResolve(); | ||
|
|
||
| expect(logHmmmSpy).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
This test pins the re-check. I'd replace it with the merge case, which fails on 79030906. It needs import createDeferredTask from '../../lib/createDeferredTask';.
| it('should not report when the value reached the cache before storage answered', async () => { | |
| await StorageMock.setItem(ONYXKEYS.TEST_KEY, 'from-storage'); | |
| forgetValueButKeepKey(ONYXKEYS.TEST_KEY); | |
| onyxSubscriptionManager.getState(ONYXKEYS.TEST_KEY); | |
| cache.set(ONYXKEYS.TEST_KEY, 'from-storage'); | |
| await waitForPromisesToResolve(); | |
| expect(logHmmmSpy).not.toHaveBeenCalled(); | |
| }); | |
| it('should report when a merge lands on the lost value before storage answers', async () => { | |
| await StorageMock.setItem(ONYXKEYS.TEST_KEY, {a: 1, b: 2}); | |
| forgetValueButKeepKey(ONYXKEYS.TEST_KEY); | |
| const getItemMock = StorageMock.getItem as jest.Mock; | |
| const readStorage = getItemMock.getMockImplementation() as (key: OnyxKey) => Promise<unknown>; | |
| const storageAnswer = createDeferredTask(); | |
| getItemMock.mockImplementationOnce((key: OnyxKey) => { | |
| const storedValue = readStorage(key); | |
| return storageAnswer.promise.then(() => storedValue); | |
| }); | |
| onyxSubscriptionManager.getState(ONYXKEYS.TEST_KEY); | |
| await Onyx.merge(ONYXKEYS.TEST_KEY, {c: 3}); | |
| storageAnswer.resolve(); | |
| await waitForPromisesToResolve(); | |
| expect(cache.get(ONYXKEYS.TEST_KEY)).toEqual({c: 3}); | |
| expect(logHmmmSpy).toHaveBeenCalledTimes(1); | |
| }); |
Details
Follow-ups to #850 from the review of Expensify/App#102135 (Expensify/App#102135 (review)).
getState(undefined)no longer throws. The cache-miss check now tests the key index beforeisRamOnlyKey, whose string parsing crashed on a non-string key. Restores the pre-3.0.119 contract: unknown or non-string key returnsundefined. Behavioral, covered bytests/unit/OnyxSubscriptionManagerTest.ts"should return undefined without throwing for an undefined key".hmmm. Covered by "should not report when the value reached the cache before storage answered". The early per-collection mark stays on purpose. It caps storage at one read per collection per session, and the miss branch runs on every render of a subscriber whose key is missing from the cache, so it has to stay at a fewSetlookups. The price is that a first member with empty storage hides a later real miss in that collection for the session. Onehmmmper collection already tells us where to look, and a real miss means storage holds a value the cache lost, which no Onyx write path produces. Better missing that over adding reads to the render path. I would keep as it is and observe logs.Onyx.getandOnyx.multiGetdrop a?? undefinedthat could never fire. The cache never storesnull, so the read returns the same values as before. Mechanical, covered bytests/unit/onyxTest.tsdescribe('get')anddescribe('multiGet')missing-key cases.Related Issues
Expensify/App#101910
Follow up after review Expensify/App#102135 (review)
Linked E/App PR
Expensify/App#104992
Automated Tests
Manual Tests
Test 1: Smoke, normal use produces no cache-miss logs
[Onyx] Cache held no value for a key storage still hasline and no[Onyx] Could not check storageline in the console.Test 2: Undefined key resolves instead of alerting
await Onyx.get()with no argument.undefined.[alrt] Unhandled Promise Rejectionline and noTypeErrormentioninglastIndexOfappears. On 3.0.119 to 3.0.123 both appear.await Onyx.multiGet([undefined])and confirm it resolves[undefined].Author Checklist
### Related Issuessection above### Linked E/App PRsection above, and verified this change against it (E/App CI passed and manual testing completed)TestssectiontoggleReportand notonIconClick)myBool && <MyComponent />.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Avataris modified, I verified thatAvataris working as expected in all cases)mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
web.test.1.mov